feat: native examine diagnose & fix - #39
Conversation
93bdba6 to
a89b881
Compare
|
@volen-silo for the sake of clarity and simplicity could we call the commands as such:
It seems a bit odd to have diagnose and fix as flags under examine since they serve a different purpose and are separate modules. |
There was a problem hiding this comment.
Requesting changes. Full review done. This is a Python→Rust port of the rocm-doctor skill (examine / diagnose / fix), and the core of it is solid: all 15 catalog checks, their scoring/keyword tables, the no-match upstream routing, and the "exactly four auto-applicable fixes" classification match the skill 1:1, the JSON contract is frozen by a test, and CI is green. The issues below are one crash bug, a few behavioral gaps against the skill's documented contract, and some repo hygiene. The first four are blocking.
Blocking
1. Crash: UTF-8 slice panic in probe_env (examine.rs:1091)
format!("{}...[truncated]", &value[..4000])value.len() > 4000 and &value[..4000] are byte offsets. If PATH or LD_LIBRARY_PATH is over 4000 bytes and byte 4000 lands inside a multibyte character (any accented or non-ASCII path component — realistic on localized systems and CI with deep dependency dirs), Examination::probe() panics and the process aborts (exit 101) instead of returning a clean exit code.
Reproduced on the built binary:
thread 'main' panicked at crates/rocm-core/src/examine.rs:1091:47:
end byte index 4000 is not a char boundary; it is inside 'é' (bytes 3999..4001 of string)
This also breaks the probe() "never fails" contract, and it's a divergence from the script — Python's value[:4000] slices by character and is safe. Suggested fix: truncate on char boundaries, e.g. value.chars().take(4000).collect::<String>(). Please add a regression test with a multibyte value crossing the boundary.
2. diagnose ignores WSL2 and emits false positives
examine detects WSL (Examination::is_wsl) and its exit_code() treats WSL as "this skill can't help here" → exit 2 (examine.rs:269-271). But diagnose never consults is_wsl: run_all_checks (diagnose.rs:1232-1247) filters checkers only by os_family, and WSL2 reports as "linux", so the entire bare-metal Linux catalog runs against a WSL2 box.
On WSL2, ROCm uses /dev/dxg + the Windows host driver, not the in-tree amdgpu module or /dev/kfd, so these misfire:
check_5_amdgpu_blacklisted—amdgpu_loaded == Some(false)is the normal state on WSL2, not a fault. Fires at score 35.check_4_render_group— render/video groups and/dev/kfdownership are irrelevant on WSL2. Fires at score 45.check_3_rocm_kernel_unsupported— theamdgpu_loaded == Some(false)branch (diagnose.rs:474-479) adds a spurious DKMS signal.
Net effect: a WSL2 user gets confident diagnoses with remediation (usermod -G render, modprobe amdgpu) that is useless-to-harmful on that platform. Reproduced via rocm diagnose on a WSL2 host (fix-6-path 70, fix-4-render-group 45, fix-5-amdgpu-load 35).
Preferred fix: short-circuit diagnose when e.is_wsl with a route-out message, consistent with exit_code() returning 2; add an is_wsl-true test asserting these no longer fire.
3. examine without --json never applies the exit code
examine() only honors examination.exit_code() inside the --json branch (main.rs:1453-1463); the text path calls render_examine_text() and always returns Ok(()) → exit 0. So on WSL / non-AMD / unsupported-platform hosts, rocm examine --json exits 2 but rocm examine exits 0. Reproduced live on WSL2 (json → 2, no-json → 0). diagnose() already exits correctly in both modes, so this is just an asymmetry in examine. The exit-code logic should apply regardless of output format.
4. examine doesn't replicate the WSL route-out note
examine.py early-returns on WSL with a note pointing at the ROCm-on-WSL install guide and runs no further probes. Examination::probe() (examine.rs:231-264) has no is_wsl branch — it runs the full Linux probe set on WSL and notes comes back empty (verified live: is_wsl: true, notes: []). The exit code is right in --json mode, but the user-facing "you're on WSL, here's where to go" guidance is lost.
Non-blocking (worth addressing or noting in the PR)
--frameworkis unreachable.FrameworkProbehasPyTorch/LlamaCpp/Skip/Auto, but the CLI exposes no flag and both handlers hardcodeAuto(main.rs:1454,1467). The skill documents--frameworkselection; only auto-detect is reachable.os_versionvalue differs. Script emitsplatform.platform()(e.g.Linux-6.8.0-…-x86_64-…); the port emitsstd::env::consts::OS→ just"linux"/"windows"(examine.rs:380). Same field name, much thinner value — not consumed bydiagnose, so the contract holds, but "field-for-field" only holds for names, not values.- Windows HIP-SDK probe is a reduced port (
examine.rs:1246-1297): omits theProgram Files (x86)scan, thenot-loadedhipInfo status, thearch:gfx fallback, version-regex extraction, and GPU-name backfill. Fine if Windows is intentionally "best-effort," but worth stating. check_10(container) scoring diverges on a nullkfd(diagnose.rs:929-932): adds +40 where the script adds 0. Converges on real probe output (the probe always populateskfd), so edge-only — a small guard or comment would close it.- Negative
--device-indexis persisted verbatim (HIP_VISIBLE_DEVICES=-1); a>= 0check would be cleaner. (No injection risk — it's ani64argv element.)
Repo hygiene
- No DCO
Signed-off-byon either commit — will fail if DCO is enforced. - Branch is behind
main— missing the OSS-cleanup commit (#20); please rebase. - PR description is stale — it says
rocm examine --diagnose/rocm examine --fix, but the final commit split these into separaterocm diagnose/rocm fixsubcommands. Please update the description.
What's solid (verified)
- All 15 checks present; scoring heuristics, keyword tables/weights, score tiers (75/50), OS gating, and
UPSTREAM_TRACKERSURLs match 1:1. - Auto-fix set is exactly
{fix-2, fix-4, fix-6, fix-9}, enforced by a test; runners buildCommandas argv vectors (no shell interpolation), print before applying, gate on consent, don't self-elevate, and never silently fall back. - The frozen top-level-keys test keeps the probe JSON and the catalog from silently diverging.
cargo clippy -p rocm-coreclean; no internal leaks in the diff.
|
Thanks @rominf — thorough review, all four blocking items addressed (plus two of the non-blocking ones). Summary: Blocking — fixed
Non-blocking — fixed
Non-blocking — deferred (noted for follow-up)
Hygiene
Re: the red |
48104e7 to
0ba3cbf
Compare
rominf
left a comment
rominf
left a comment
There was a problem hiding this comment.
Re-reviewed at 0ba3cbfe. Thanks for the fast turnaround — every change I asked for landed and I verified each one by building the binary and running it on a WSL2 host:
- ✅ UTF-8 panic in
probe_env—truncate_to_charsis char-safe; the input that previously aborted with exit 101 now exits cleanly, with two regression tests. - ✅
diagnoseWSL2 handling —out_of_scopeshort-circuit + route-out, with tests. - ✅
examinetext-mode exit code + WSL route-out note now present. - ✅ fix-10 null-
kfdedge corrected; rebased onto currentmain; PR description updated.
130 tests pass locally. Good work. One new blocking issue, and it traces back to my own previous request — so the fix is a design decision, not just a patch.
Blocking: CI is red on both platforms — the exit-code change broke a pre-existing smoke test
build-and-test and windows-build-and-test both fail with:
smoke failed: rocm examine exited with status 2
scripts/smoke_local.py:261 runs rocm examine, expects exit 0, and asserts the setup inventory (default_engine:, managed_runtimes: 0). CI runners have no AMD GPU, so the new "exit 2 when no AMD GPU" makes that command fail on every GPU-less host.
The real problem this exposes: rocm examine wears two hats. It's both the pre-existing setup inspector you run on any box (GPU or not) and the new host probe where "no AMD GPU → 2" feels right. Encoding a diagnostic verdict in the exit code breaks the inspector role — and it also collides with clap, which already uses exit 2 for usage errors. My earlier "make text mode honor the exit code" request was right about the WSL/JSON asymmetry but, taken literally, made the no-GPU case exit 2 too. That's what the smoke test caught.
Requested change: an exit-code scheme where the code reports execution, not the finding
Findings (no GPU, WSL, no match) belong in the output and --json; the exit code should only say whether the command ran. 2 stays reserved for clap; 1 is the conventional "it failed."
Shared: 0 ran · 1 internal error · 2 usage (clap).
rocm examine (reporter): 0 for any finding — GPU, no GPU, WSL, degraded — surfaced via output and a new --json status field (ok / no-amd-gpu / wsl / unsupported-os / degraded). 1 only if it genuinely can't examine. This is what turns the smoke test green by design rather than by relaxing the test.
rocm diagnose (query): 0 whether it matched, found nothing, or is out of scope — callers read has_match / out_of_scope / route_when_no_match from --json. (Your WSL tests already assert out_of_scope.is_some() + !has_match(), which are unaffected — only the exit-code expectation flips.)
rocm fix (the only command that acts, so the only one with richer states): 0 applied / dry-run / list / print-only plan · 1 internal error · 2 usage incl. unknown fix-id · 3 not applicable on this host (OS mismatch, missing --device-index) — refused, nothing changed · 4 attempted but failed.
Net: agents branch on the JSON (status, out_of_scope, has_match), never on the exit integer — which is strictly richer than the old 2/3 codes. One line in the skill/PR noting that migration covers it; the --json field contract the PR claims is untouched (we add status, remove nothing).
If you'd rather keep rocm diagnose && … working as a shell predicate, the grep model (0 match / 1 no-match / 2 error) is also defensible — but then out-of-scope needs its own code (3) so WSL ≠ no-match, and examine must still stay 0 on the GPU-less box. I'd lean toward the uniform scheme above for consistency.
Minor (non-blocking)
- DCO sign-off is on only 1 of the 4 commits — the other three lack
Signed-off-by; will fail per-commit DCO. --frameworkis still unreachable at the CLI (enum exists, handlers hardcodeAuto). Fine to defer, but worth a note in the PR if it's intentional.
0ba3cbf to
c6126d2
Compare
There was a problem hiding this comment.
Pull request overview
Adds native rocm examine, rocm diagnose, and rocm fix functionality by porting the rocm-doctor probe/closed-catalog diagnosis/fix runner into Rust, with rocm-core becoming the single source of truth for the catalog and its wire contracts.
Changes:
- Introduces new
rocm-coremodules:examine(host probe + Examination JSON),diagnose(15-check closed catalog + report rendering), andfix(consent-gated fix runners + recipe registry). - Re-exports the new APIs from
rocm-coreand adds CLI subcommands inapps/rocmto exposeexamine/diagnose/fix(including--jsonand fix options). - Adds the
regexdependency to support symptom keyword scoring and version parsing helpers in diagnosis.
Reviewed changes
Copilot reviewed 6 out of 7 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| crates/rocm-core/src/lib.rs | Exposes new examine/diagnose/fix modules and re-exports their public API for the CLI. |
| crates/rocm-core/src/examine.rs | New host probe producing the Examination JSON contract (Linux + Windows best-effort) with contract-freezing tests. |
| crates/rocm-core/src/diagnose.rs | New closed-catalog diagnosis engine with keyword scoring, routing, and text/JSON report output. |
| crates/rocm-core/src/fix.rs | New fix recipe registry plus consent-gated runners for auto-applicable fixes and listing/printing plans. |
| crates/rocm-core/Cargo.toml | Adds regex dependency required by diagnosis logic. |
| Cargo.lock | Locks regex into the workspace dependency graph. |
| apps/rocm/src/main.rs | Adds examine --json, diagnose, and fix subcommands and dispatch logic. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if let Err(exc) = append_line( | ||
| &rc_file, | ||
| "# Added by rocm examine (fix-6-path)", | ||
| &export_line, | ||
| ) { |
| if let Err(exc) = append_line( | ||
| &rc_file, | ||
| "# Added by rocm examine (fix-9-igpu-dgpu)", | ||
| &export_line, | ||
| ) { |
| fn newest_rocm_install_dir() -> String { | ||
| for root in [ | ||
| r"C:\Program Files\AMD\ROCm", | ||
| r"C:\Program Files (x86)\AMD\ROCm", | ||
| ] { | ||
| if let Ok(entries) = std::fs::read_dir(root) { | ||
| let mut versions: Vec<PathBuf> = entries | ||
| .flatten() | ||
| .map(|e| e.path()) | ||
| .filter(|p| { | ||
| p.is_dir() | ||
| && p.file_name() | ||
| .and_then(|n| n.to_str()) | ||
| .is_some_and(|n| n.chars().next().is_some_and(|c| c.is_ascii_digit())) | ||
| }) | ||
| .collect(); | ||
| versions.sort(); | ||
| if let Some(latest) = versions.last() { | ||
| return latest.to_string_lossy().into_owned(); | ||
| } | ||
| } | ||
| } | ||
| String::new() | ||
| } |
| // `rocm examine` is the general system inspector: the exit code reports | ||
| // whether it RAN, not what it found. Any finding (no GPU, WSL, degraded) is | ||
| // surfaced in the output and the `--json` `status` field, and the command | ||
| // exits 0; a genuine inability to examine propagates as an error via `?`. | ||
| if json { |
| let mut versions: Vec<String> = entries | ||
| .flatten() | ||
| .filter(|entry| entry.path().is_dir()) | ||
| .map(|entry| entry.file_name().to_string_lossy().into_owned()) | ||
| .collect(); | ||
| versions.sort(); | ||
| if let Some(latest) = versions.last() { | ||
| root = base.join(latest).to_string_lossy().into_owned(); | ||
| } |
| let new_path = if user_path.is_empty() { | ||
| bin_dir.clone() | ||
| } else { | ||
| format!("{user_path};{bin_dir}") | ||
| }; | ||
| println!("Plan: prepend {bin_dir} to your User PATH:"); | ||
| println!(" setx PATH \"{new_path}\""); |
Add the rocm-doctor capability to the rocm binary: probe the host, diagnose against a closed catalog of known ROCm/PyTorch/llama.cpp misconfigurations, and apply consent-gated fixes. The probe, the closed catalog (checks, keyword tables, scoring), and the fix recipes live in rocm-core as one source of truth, versioned with the binary and usable standalone -- no external scripts or agent required. - examine: structured Examination host probe (Linux full parity, Windows best-effort), with a machine-readable JSON form for tooling - diagnose: the 15 closed-catalog checks with evidence, a fix, a verify step, and upstream routing when nothing matches - fix: consent-gated runners for the four safe fixes; risky fixes print their plan and mutate nothing A field-set test freezes the Examination JSON contract so the probe output and the catalog cannot silently diverge. Signed-off-by: Eugene Volen <Eugene.Volen@amd.com>
Address review feedback: diagnose and fix are distinct verbs backed by separate modules, so expose them as top-level commands rather than mode flags under `examine`. - rocm examine [--json] host probe / report - rocm diagnose [--symptom][--top][--json] match the closed catalog - rocm fix [<id>][--yes][--dry-run][--device-index] apply or list fixes Also register diagnose/fix in the natural-language allowlist so they dispatch as structured commands instead of falling through to the freeform planner. Help strings updated to the new command names. Signed-off-by: Eugene Volen <Eugene.Volen@amd.com>
…heme
- probe_env: char-boundary truncation for PATH/LD_LIBRARY_PATH so a
multibyte char at the truncation cut no longer panics probe(); the cap is
a named constant (16k chars) chosen well past any realistic ROCm bin entry
so the fix-6 PATH check isn't tripped by truncation. Regression test added.
- Exit codes report execution, not findings (per review):
- examine: always exits 0 (a genuine inability to examine propagates as
an error). The verdict is a --json `status` field
(ok / no-amd-gpu / wsl / unsupported-os / degraded). WSL2 also skips the
Linux probe set and shows a route-out note.
- diagnose: always exits 0; callers read has_match / out_of_scope /
route_when_no_match from --json. WSL2 short-circuits with out_of_scope.
- fix: 0 ok/dry-run/list/print, 1 internal error, 2 usage incl. unknown
fix-id, 3 not applicable (OS mismatch / missing or negative
--device-index), 4 attempted-but-failed, 5 user declined.
- check-10 (container): a null kfd contributes 0.
Adds is_wsl tests for diagnose, status-precedence and multibyte-truncation
tests for examine.
Signed-off-by: Eugene Volen <Eugene.Volen@amd.com>
c6126d2 to
0757b12
Compare
Add the rocm-doctor capability — host examination, diagnosis against a closed catalog of known misconfigurations, and consent-gated fixes — to the
rocmbinary as native subcommands. The catalog and probe live in the CLI as one source of truth, versioned with the binary and usable standalone (no Python or agent required). Theamd/skillsrocm-doctor skill then becomes a thin layer that just invokes these commands.Commands
rocm examine [--json]— host probe / report.--jsonemits the structured Examination document (Linux full parity, Windows best-effort) for tooling. It's a general system inspector, so it always exits 0; the verdict is reported in output and a--jsonstatusfield (ok/no-amd-gpu/wsl/unsupported-os/degraded). WSL2 is reported with route-out guidance.rocm diagnose [--symptom "…"] [--top N] [--json]— match the host against the 15 closed-catalog failure modes; returns ranked causes with evidence, a fix, a verify step, and upstream routing when nothing matches. Always exits 0; callers readhas_match/out_of_scope/route_when_no_matchfrom--json. WSL2 short-circuits as out of scope.rocm fix [<id>] [--yes] [--dry-run] [--device-index N]— apply a known fix (the four safe, auto-applicable ones); risky fixes print their plan and mutate nothing. Run with no id to list fixes. Exit codes:0ran ·1internal error ·2usage incl. unknown id ·3not applicable on this host ·4attempted but failed ·5declined.Design
rocm-core(examine/diagnose/fixmodules).ExaminationJSON contract so the probe output and the catalog can't silently diverge.status,has_match,out_of_scope).2stays reserved for clap usage errors.Notes / follow-ups
/dev/dxg+ the Windows host driver, not theamdgpumodule or/dev/kfd), so the bare-metal catalog would only produce false positives. This PR detects it and routes out (status: "wsl"/out_of_scope); the follow-up will add a WSL-specific probe + catalog.--frameworkselection is not yet wired at the CLI (probes default to auto-detect).os_versionis the coarsestd::env::consts::OSvalue (not consumed by diagnosis).